Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(deps): Reduce FSharp.Core dependency back to 5.0.0 #184

Closed
wants to merge 5 commits into from
Closed

fix(deps): Reduce FSharp.Core dependency back to 5.0.0 #184

wants to merge 5 commits into from

Conversation

bartelink
Copy link
Contributor

@bartelink bartelink commented Feb 28, 2024 •
edited
Loading

FsHttp package versions 13-14.4.0 omitted to state their dependency on FSharp.Core

This means that anyone who is using a FSharp.Core prior to 8.0.100 (which v 14.4.1 correctly declares its dependence on) winds up (if they use a tool that depends on nuspec dependency declarations (such as paket, but also Rider etc):

  • getting 14.4.0 (technically correct, not a paket bug)
  • (on a machine that is running a net6.0 runtime) getting surprised at runtime with a: 
    System.IO.FileLoadException: Could not load file or assembly 'FSharp.Core, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
The located assembly's manifest definition does not match the assembly reference. (0x80131040)
File name: 'FSharp.Core, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

This PR rolls all the way back to only demanding V5, like the older versions did

resolves #183

@bartelink bartelink changed the title fix(deps): Reduce min FSharp.Core dependency back to 5.0.0 fix(deps): Reduce FSharp.Core dependency back to 5.0.0 Feb 28, 2024
@bartelink bartelink mentioned this pull request Feb 28, 2024
Merged
@dawedawe
Copy link
Contributor

Mmh, I'm not sure about this one.
If I remember correctly we made the move to FSharp.Core 8 because @SchlenkR had other build issues with the dependency on 5.
But you're right, the 14.0 release was bad because of the implicit dependency. That was my fuckup.

@bartelink
Copy link
Contributor Author

But... it builds fine?

This scheme/policy is used in lots of places, e.g. Argu, github/jet

Ultimately, apps will typically have an implicit ref to the latest FSharp.Core, and are in a position to control that, and aside from not inducing risk by depending on a known broken/blacklisted thing like e.g. Newtonsoft < 13.0.3, it's just not something that a library should be dictating

Right now, people in the same boat as was (actually, a colleague) can fall victim to this. The remedies (aside from hoping they hit this via google) are one of:

  • go this road
  • unlist all the broken nugets (v12.1.0-v14.0.0)

@SchlenkR
Copy link
Owner

SchlenkR commented Mar 5, 2024

@bartelink I only had a look at the issue; not at your PR. You basically did the work I finished right now. Please excuse me for not merging your PR; I would have done it if I had seen it before.

Thank you so much for your PR and all the information. This is so valuable, since such topics usually cost nerves and energy. Great stuff - 🙏❤️

@SchlenkR SchlenkR closed this Mar 5, 2024
@bartelink
Copy link
Contributor Author

Thanks @SchlenkR

One straggler piece is that the comments don't align with the PackageRef now, will make a PR

https://github.com/fsprojects/FsHttp/pull/184/files#diff-2c1498892796bdfa6445f171ee3c2c11cae3b082c3c6896cdb2d4e275bbda3ffR41-R48

This is not critical - you've already done all the important stuff, and it's much appreciated...

@bartelink bartelink mentioned this pull request Mar 5, 2024
Merged
@bartelink bartelink deleted the minimize-deps branch March 5, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FSharp.Core dependency confusion
3 participants
@bartelink @dawedawe @SchlenkR